Introduce RenegotiatedFunding monitor update variant #3822

wpaulino · 2025-06-02T23:53:04Z

This is a new ChannelMonitorUpdateStep variant intended to be used whenever a new funding transaction for the channel has been negotiated via the InteractiveTxConstructor. This commit primarily focuses on its use for splices, but future work will expand where needed to support RBFs (both for the initial dual funding transaction, and splice transactions).

To draw a parallel to channel open, we generally want to have the commitment transactions negotiated for the funding transaction and committed to the respective ChannelMonitor before attempting to sign the funding transaction itself. This monitor update fulfills this need for a newly negotiated splice; it includes both the new holder and counterparty commitment transactions, and the new set of applicable ChannelTransactionParameters. Once the monitor update has been applied to the monitor and persisted, we allow the release of our tx_signatures for the splice transaction to wait for its confirmation.

Depends on #3774.

ldk-reviews-bot · 2025-06-02T23:53:07Z

👋 I see @TheBlueMatt was un-assigned.
If you'd like another reviewer assignemnt, please click here.

codecov · 2025-06-03T00:48:44Z

Codecov Report

Attention: Patch coverage is 35.61644% with 188 lines in your changes missing coverage. Please review.

Project coverage is 89.63%. Comparing base (db165ad) to head (180b53d).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	11.49%	154 Missing ⚠️
lightning/src/ln/channel.rs	81.18%	15 Missing and 4 partials ⚠️
lightning/src/chain/chainmonitor.rs	11.76%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3822      +/-   ##
==========================================
- Coverage   89.74%   89.63%   -0.12%     
==========================================
  Files         164      164              
  Lines      133079   134658    +1579     
  Branches   133079   134658    +1579     
==========================================
+ Hits       119437   120694    +1257     
- Misses      10960    11275     +315     
- Partials     2682     2689       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-06-03T23:26:05Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

lightning/src/ln/channel.rs

tankyleo

First pass in channel monitor

lightning/src/chain/channelmonitor.rs

ldk-reviews-bot · 2025-06-11T00:35:24Z

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

stared at the code again today, picked up nits on the way.

lightning/src/chain/channelmonitor.rs

lightning/src/chain/chainmonitor.rs

TheBlueMatt · 2025-06-11T14:58:58Z

lightning/src/ln/channel.rs

@@ -1677,7 +1677,33 @@ impl<SP: Deref> Channel<SP> where
 				res
 			},
 			ChannelPhase::Funded(mut funded_channel) => {
-				let res = funded_channel.commitment_signed(msg, logger).map(|monitor_update_opt| (None, monitor_update_opt));
+				#[cfg(splicing)]


Sheesh, is it too late to make this a new damn message? We now have three totally separate handling logic for commitment_signed, and this one doesn't even make any sense - we're receiving a single commitment_signed but we actually need two if we were in a normal state...

Sheesh, is it too late to make this a new damn message?

Probably not? Would be a trivial thing to change, but it would basically be a copy of the commitment_signed message.

this one doesn't even make any sense - we're receiving a single commitment_signed but we actually need two if we were in a normal state...

I'm not following, what do you mean by "normal state"? This is handling the case where we haven't finished negotiating a splice transaction. We expect a single commitment_signed to be exchanged for the alternative commitments at the current commitment number. After the negotiation finishes, if we're expecting a batch and they didn't send it, we'll fail the channel. If we're not expecting a batch and they did send it, we'll also fail the channel.

TheBlueMatt · 2025-06-11T15:00:28Z

lightning/src/ln/channel.rs

+			pending_splice_funding, &self.holder_commitment_point, msg, logger,
+		)?;
+
+		let counterparty_commitment_tx = self.context.build_commitment_transaction(


Bleh, we already build the commitment tx in validate_commitment_signed, lets not do it again.

We need to build both, validate_commitment_signed builds the holder one, this builds the counterparty one

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-06-14T00:00:05Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-16T00:00:59Z

🔔 3rd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-16T00:01:18Z

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2025-06-17T14:37:54Z

Needs rebase, it seems.

TheBlueMatt

Basically LGTM

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2025-06-18T14:39:21Z

LGTM, I think, modulo the comment request above @jkczyz or @tankyleo can take a look now.

tankyleo

Code all LGTM pending Matt's last unresolved comment above.

@jkczyz would you be able to take a look at channel.rs on this PR ? Code makes sense to me, but not as confident with the p2p splicing dance :)

ldk-reviews-bot · 2025-06-23T00:01:06Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2025-06-23T15:51:49Z

Did you want to take a look, @jkczyz? Given @tankyleo didn't ACK above.

lightning/src/ln/channel.rs

jkczyz · 2025-06-23T16:21:58Z

lightning/src/ln/channel.rs

+					funded_channel
+						.splice_initial_commitment_signed(msg, logger)
+						.map(|monitor_update_opt| (None, monitor_update_opt))
+				} else {


We'll hit this when has_negotiated_pending_splice && session_received_commitment_signed are true when we'd be expecting a batch. But I guess the checks in FundedChannel::commitment_signed will catch this. Where refactoring pending_splice and pending_funding, I can see if we'd want to re-organize any of that logic.

Yeah that's how I noticed that we were missing the check that you recently added.

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

jkczyz · 2025-06-23T16:35:22Z

lightning/src/chain/channelmonitor.rs

+			// Only one splice can be negotiated at a time after we've exchanged `channel_ready`
+			// (implying our funding is confirmed) that spends our currently locked funding.
+			if !self.pending_funding.is_empty() {


Won't this be non-empty once we support RBF?

Correct, this commit doesn't make any assumptions based on RBF though, that's why I added the TODO above.

This is a new `ChannelMonitorUpdateStep` variant intended to be used whenever a new funding transaction for the channel has been negotiated via the `InteractiveTxConstructor`. This commit primarily focuses on its use for splices, but future work will expand where needed to support RBFs (both for the initial dual funding transaction, and splice transactions). To draw a parallel to channel open, we generally want to have the commitment transactions negotiated for the funding transaction and committed to the respective `ChannelMonitor` before attempting to sign the funding transaction itself. This monitor update fulfills this need for a newly negotiated splice; it includes both the new holder and counterparty commitment transactions, and the new set of applicable `ChannelTransactionParameters`. Once the monitor update has been applied to the monitor and persisted, we allow the release of our `tx_signatures` for the splice transaction to wait for its confirmation.

TheBlueMatt

Two minor notes that might be worth following up on.

TheBlueMatt · 2025-06-24T01:18:17Z

lightning/src/chain/chainmonitor.rs

+						);
+						chain_source.register_tx(&funding_outpoint.txid, &funding_script);
+						chain_source.register_output(WatchedOutput {
+							block_hash: None,


I'm not sure any downstream applications use it, but we could fill this in with the current chain tip, AFAIU? (and honestly rename the field while we're at it lol)

Unfortunately this would only last until the next restart (assuming the chain_source is not persisting WatchedOutputs), so we'd have to track the block hash if we always want to set it.

TheBlueMatt · 2025-06-24T01:26:54Z

lightning/src/chain/channelmonitor.rs

+		let redeem_script = channel_parameters.make_funding_redeemscript();
+
+		Ok(Self {
+			script_pubkey: redeem_script.to_p2wsh(),


AFAICT script_pubkey is only used once on startup in load_outputs_to_watch and otherwise in the actually-unused get_funding_script. ISTM we could just drop both. That said, even if we don't, we should still be able to condense the Readable and Writeables here with a static_value calculating the redeem_script and script_pubkey.

Added a commit for this in #3855

wpaulino requested a review from jkczyz June 2, 2025 23:53

jkczyz reviewed Jun 3, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

jkczyz reviewed Jun 5, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 6c78591 to ac8c8c5 Compare June 9, 2025 00:35

wpaulino requested a review from jkczyz June 9, 2025 00:35

tankyleo self-requested a review June 9, 2025 19:55

tankyleo reviewed Jun 9, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from ac8c8c5 to 5f0709f Compare June 10, 2025 18:58

wpaulino requested a review from TheBlueMatt June 10, 2025 18:58

tankyleo reviewed Jun 11, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jun 11, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 5f0709f to 689d0c1 Compare June 11, 2025 17:04

wpaulino requested a review from TheBlueMatt June 12, 2025 17:23

wpaulino mentioned this pull request Jun 12, 2025

Introduce splice-compatible commitment update monitor variants #3855

Open

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 689d0c1 to 4a38fd2 Compare June 13, 2025 17:20

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 4a38fd2 to b274f78 Compare June 16, 2025 17:13

TheBlueMatt reviewed Jun 17, 2025

View reviewed changes

wpaulino force-pushed the renegotiated-funding-monitor-update branch from b274f78 to a201837 Compare June 17, 2025 17:38

wpaulino requested review from TheBlueMatt and tankyleo and removed request for jkczyz June 17, 2025 17:39

TheBlueMatt reviewed Jun 17, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from a201837 to a94ac4c Compare June 18, 2025 01:55

wpaulino requested a review from TheBlueMatt June 18, 2025 01:56

TheBlueMatt removed their request for review June 18, 2025 14:39

tankyleo reviewed Jun 18, 2025

View reviewed changes

wpaulino force-pushed the renegotiated-funding-monitor-update branch from a94ac4c to 75a7cb6 Compare June 20, 2025 19:26

wpaulino requested a review from TheBlueMatt June 20, 2025 19:27

jkczyz mentioned this pull request Jun 21, 2025

Dual-funded channels and Splicing Project Tracking #1621

Open

7 tasks

TheBlueMatt removed their request for review June 23, 2025 15:51

jkczyz reviewed Jun 23, 2025

View reviewed changes

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 75a7cb6 to 180b53d Compare June 23, 2025 17:26

jkczyz approved these changes Jun 23, 2025

View reviewed changes

TheBlueMatt approved these changes Jun 24, 2025

View reviewed changes

TheBlueMatt merged commit c48e0a8 into lightningdevkit:main Jun 24, 2025
26 of 28 checks passed

wpaulino deleted the renegotiated-funding-monitor-update branch June 24, 2025 02:18

Introduce RenegotiatedFunding monitor update variant #3822

Introduce RenegotiatedFunding monitor update variant #3822

Conversation

wpaulino commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 3, 2025

Uh oh!

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 11, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 14, 2025

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 17, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jun 23, 2025

Uh oh!

TheBlueMatt commented Jun 23, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpaulino commented Jun 2, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 2, 2025 •

edited

Loading

codecov bot commented Jun 3, 2025 •

edited

Loading

TheBlueMatt commented Jun 18, 2025 •

edited

Loading